SED-4536 granular-access-control-for-resource-types by david-stephan · Pull Request #617 · exense/step

david-stephan · 2026-03-20T10:10:48Z

No description provided.

gemini-code-assist · 2026-03-20T10:11:11Z

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request implements granular access control for resource types, enabling more precise permission management. Instead of broad 'resource-read' or 'resource-write' rights, permissions can now be defined for specific resource types, such as 'resource-image-read' or 'resource-document-write'. This enhancement improves security and flexibility by allowing administrators to tailor access based on the nature of the resource.

Highlights

Granular Access Control: Introduced granular access control for resource types, allowing permissions to be defined at the resource-<type>-<action> level.
RoleProvider Enhancement: Updated the RoleProvider interface to include a method for retrieving all available rights.
Access Control Utilities: Implemented new utility methods and constants in AbstractStepServices to support dynamic right checking.
Resource Services Integration: Modified ResourceServices to enforce these new granular access checks across various resource operations, including creation, saving, retrieval, and deletion.
Resource Object Exposure: Enhanced ResourceRevisionContent to directly expose the associated Resource object.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces granular access control for resource types. The changes primarily involve adding permission checks in ResourceServices based on the resource type for various operations. My review has identified a critical security vulnerability in the saveResource method that could allow bypassing these new security checks. I have also noted a couple of medium-severity issues concerning exception handling and a potentially risky "fail-open" design choice. Please see my detailed comments below.

gemini-code-assist · 2026-03-20T10:13:20Z

step-controller/step-controller-server/src/main/java/step/resources/ResourceServices.java

    @Consumes(MediaType.APPLICATION_JSON)
    @Produces(MediaType.APPLICATION_JSON)
    public Resource saveResource(Resource resource) throws IOException {
+        checkResourceTypeRight(resource, WRITE_RIGHT);


The current implementation has a security vulnerability. A user could update a resource and remove its resourceType to bypass granular access controls. To prevent this, you must check permissions against the resource's currently stored state before applying changes.

Additionally, for consistency with createResource, saveResource should enforce that newly created resources have a resourceType.

if (resource.getId() != null) { // This is an update, check against the stored resource Resource storedResource = resourceManager.getResource(resource.getId().toHexString()); checkResourceTypeRight(storedResource, WRITE_RIGHT); } else { // This is a new resource, type is mandatory if (resource.getResourceType() == null || resource.getResourceType().isEmpty()) { throw new RuntimeException("Missing resourceType for new resource"); } } checkResourceTypeRight(resource, WRITE_RIGHT);

Checking now access to both types (current stored in DB and new one), also ensure types is set and resource is not null

gemini-code-assist · 2026-03-20T10:13:20Z

step-controller/step-controller-server/src/main/java/step/resources/ResourceServices.java

+    private void checkResourceTypeRight(Resource resource, String right) {
+        //We have different services where we do not explicitly check if the type is set, for this reason that if type is not set we grant access
+        String resourceType = resource.getResourceType();
+        if (resourceType != null && !resourceType.isEmpty()) {
+            checkResourceTypeRight(resourceType, right);
+        }
+    }


This method grants access if a resource has no resourceType. While the comment explains this is intentional, this "fail-open" behavior can be a security risk. If a resource is created or modified to have no type (either intentionally by a malicious user or accidentally due to a bug), granular access control is bypassed.

For improved security, consider a "fail-closed" approach where access is denied for resources without a type, or at least log a warning when such resources are accessed to detect potential issues.

@jeromecomte I did this on purpose, because many of our services don't check at all if a resourceType is set or not. If you think it is fine from now on to throw an exception when a resource as no resourceType, I'll make the modification.

@david-stephan This would be safer but we should ensure that we don't break anything. Do you know which clients could call this without resourceType?

Well I was hoping you would have an idea :). Now I deep dived into the code, createResource ensure a type is set since 2019 at least. saveResource doesn't but it is apparently only called from the UI which enforce providing a type. The RemoteResourceManager does not use this service, the method saveResource is "Not implemented". The CLI/mvn plugin use the AP services directly. So apart from custom REST clients, no client is supposed to update an existing resource removing the type.

gemini-code-assist · 2026-03-20T10:13:20Z

step-controller/step-controller-server/src/main/java/step/resources/ResourceServices.java

+            try {
+                checkResourceTypeRight(r, READ_RIGHT);
+                return true;
+            } catch (Exception e) {


Catching a generic Exception is not ideal as it can hide other unexpected errors. It's better to catch the specific AuthorizationException that checkResourceTypeRight is expected to throw on a permission failure. This makes the code more robust.

} catch (AuthorizationException e) {

jeromecomte · 2026-03-20T11:21:44Z

step-controller/step-controller-server/src/main/java/step/resources/ResourceServices.java

+
+    private void checkResourceTypeRight(Resource resource, String right) {
+        //We have different services where we do not explicitly check if the type is set, for this reason that if type is not set we grant access
+        String resourceType = resource.getResourceType();


The resource could be null in some services. Not sure how we should propertly handle this in terms of right validation but we shouldn't throw an NPE here

The only place it could be null is for the saveResource service if the payload is empty. In other cases, a MissingResourceException is thrown before entering this method. To be clean and safe I not added a validation and throw an exception.

jeromecomte · 2026-03-20T19:39:30Z

step-controller/step-controller-server/src/main/java/step/resources/ResourceServices.java

-        String resourceType = resource.getResourceType();
-        if (resourceType != null && !resourceType.isEmpty()) {
-            checkResourceTypeRight(resourceType, right);
+        //If a resource is null, right is granted too, it's not the role of this function to perform integrity check


The comment doesn't seem to match the implementation

jeromecomte · 2026-03-20T19:44:44Z

step-controller/step-controller-server/src/main/java/step/resources/ResourceServices.java

-            checkResourceTypeRight(resourceType, right);
+        //If a resource is null, right is granted too, it's not the role of this function to perform integrity check
+        if (resource == null) {
+            throw new ControllerServiceException(INVALID_ARGUMENTS_A_NON_NULL_RESOURCE_MUST_BE_PROVIDED);


This exception might be misleading. In some case it might mean that the provided resourceId doesn't exist (61a162b#diff-3e91bc97d77a8308abd81e9cdf8a6c793ab84adaf260c3100ebf11f6959c6e12R165)

david-stephan added 2 commits March 19, 2026 17:35

SED-4536 Granular access control for resource types

ff12ac3

SED-4536 revert right setting for resources table

4033942

gemini-code-assist bot reviewed Mar 20, 2026

View reviewed changes

jeromecomte requested changes Mar 20, 2026

View reviewed changes

SED-4536 PR feedbacks

61a162b

jeromecomte reviewed Mar 20, 2026

View reviewed changes

Conversation

david-stephan commented Mar 20, 2026

Uh oh!

gemini-code-assist bot commented Mar 20, 2026

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants